-
Notifications
You must be signed in to change notification settings - Fork 895
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handle plugin registration failure ContextualVersionConflict with log instead of raising error #1542
Handle plugin registration failure ContextualVersionConflict with log instead of raising error #1542
Conversation
Signed-off-by: noklam <nok.lam.chan@quantumblack.com>
Signed-off-by: noklam <nok.lam.chan@quantumblack.com>
Signed-off-by: noklam <nok.lam.chan@quantumblack.com>
Signed-off-by: noklam <nok.lam.chan@quantumblack.com>
Signed-off-by: noklam <nok.lam.chan@quantumblack.com>
pkg_resources
with importlib_metadata
for entrypoints discovery
Signed-off-by: noklam <nok.lam.chan@quantumblack.com>
Signed-off-by: noklam <nok.lam.chan@quantumblack.com>
Signed-off-by: noklam <nok.lam.chan@quantumblack.com>
pkg_resources
with importlib_metadata
for entrypoints discoverySigned-off-by: noklam <nok.lam.chan@quantumblack.com>
Signed-off-by: noklam <nok.lam.chan@quantumblack.com>
Signed-off-by: noklam <nok.lam.chan@quantumblack.com>
Signed-off-by: noklam <nok.lam.chan@quantumblack.com>
Signed-off-by: noklam <nok.lam.chan@quantumblack.com>
kedro/framework/cli/utils.py
Outdated
except Exception as exc: | ||
raise KedroCliError(f"Loading {name} commands from {entry_point}") from exc | ||
except Exception as exc: # pylint: disable=broad-except | ||
logger.warning( | ||
KedroCliError(f"Fail to load {name} commands from {entry_point}") | ||
) | ||
logger.warning(exc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the most important part. It's hard to determine from the framework side whether a kedro's run is still valid if plugins fails. kedro-viz
is defintely ok to just ignore it, but for kedro-mlflow
type of plugin that actually modify the run behavior, it may be problematic so I am using warning
level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar suggestion here to the other occurrence in cli.py.
pyproject.toml
Outdated
# PEP-518 https://peps.python.org/pep-0518/ | ||
[build-system] | ||
# Minimum requirements for the build system to execute. | ||
requires = ["setuptools>=38.0", "wheel"] # PEP 508 specifications. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is something I just come across when I work on this PR, no related to the PR particularly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've got "setuptools>=38.0"
and wheel
requirements in various other places in the repo (just do a Ctrl+F to see them). Does this mean we can remove them from there?
If yes then that would be nice but a bit of a bigger change, so probably worth doing in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, created a new PR and will revert this soon.
@@ -299,37 +299,44 @@ def test_project_groups(self, entry_points, entry_point): | |||
entry_point.load.return_value = "groups" | |||
groups = load_entry_points("project") | |||
assert groups == ["groups"] | |||
entry_points.assert_called_once_with(group="kedro.project_commands") | |||
entry_points.return_value.select.assert_called_once_with( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the test, it's mainly just catching the log instead of catching the error. Due to the entry_points
API, I have to mock 2 layers which makes the assertion a bit longer.
I still have to make it work for py37... but feel free to drop comments. |
Signed-off-by: noklam <nok.lam.chan@quantumblack.com>
…p-kedro-running-when-dependency-clashes
Signed-off-by: noklam <nok.lam.chan@quantumblack.com>
…ct-error-stop-kedro-running-when-dependency-clashes' into fix/1487-contextualversionconflict-error-stop-kedro-running-when-dependency-clashes Signed-off-by: noklam <nok.lam.chan@quantumblack.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Another ticket which turned out to be much harder than expected, great job tackling it ⭐
I think a more conventional way to deal with a backported package would be
- try to import
importlib.metadata
and only if that failsimport importlib_metadata
- add
python_version < '3.8'
toimportlib_metadata
in requirements so that it's only installed if necessary
See, for example, https://github.com/pallets/click/pull/1890/files.
But I don't know if this is possible here because of the API change you mention 😬
@@ -65,10 +68,9 @@ def info(): | |||
plugin_versions = {} | |||
plugin_entry_points = defaultdict(set) | |||
for plugin_entry_point, group in ENTRY_POINT_GROUPS.items(): | |||
for entry_point in pkg_resources.iter_entry_points(group=group): | |||
for entry_point in importlib_metadata.entry_points().select(group=group): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this also raise some kind of exception? Or is it just the entry_point.load
that we should wrap in the try
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though I guess this is just kedro info
rather than being triggered on every single kedro
command like the other instances so it doesn't matter if there's uncaught exceptions anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is indeed the solution I go with initially, but later I find out the API was not consistent. So technically only Python 3.10 can use the stdlib.
Originally I have this conditional import block in kedro.utils
, otherwise I have to do this conditionally everywhere. Do you think it is the right place to do so? 3c12bcc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could fail, but like you said it should be just kedro info
, unlike other plugin where the program still run without loading it. I think it is ok to leave it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Urghh, what a pain. I think what you did in 3c12bcc was ok, although probably it's more usual to just repeat the conditional import in multiple files.
BUT if the standard library API is only right in Python 3.10 then let's just forget about it and go for importlib_metadata
all the way like you're doing now 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have considered that, but it also means we have to copy the same block in multiple files, and also anywhere that use mocker.patch
, which is quite hard to read.
For the context, I started this PR with importlib_metadata
, so I wasn't aware of the inconsistent API between the Python version. In theory, if I avoid using the select
API, it could be compatible with python3.8-3.10. But for both case, we still need to have the conditional import, so I would rather just stick with importlib_metadata
, unless this extra dependencies is causing trouble.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that totally makes sense. I'm definitely happy to go with importlib_metadata
throughout. It's not worth the extra complexity doing it with standard library only for Python 3.10.
kedro/framework/cli/cli.py
Outdated
logger.warning(KedroCliError(f"Fail to initialize {entry_point}")) | ||
logger.warning(exc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logger.warning(KedroCliError(f"Fail to initialize {entry_point}")) | |
logger.warning(exc) | |
logger.warning(f"Failed to initialise %s. Full exception: %s", entry_point, exc) |
Unless there's as good reason to use the KedroCliError
still?
kedro/framework/cli/utils.py
Outdated
except Exception as exc: | ||
raise KedroCliError(f"Loading {name} commands from {entry_point}") from exc | ||
except Exception as exc: # pylint: disable=broad-except | ||
logger.warning( | ||
KedroCliError(f"Fail to load {name} commands from {entry_point}") | ||
) | ||
logger.warning(exc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar suggestion here to the other occurrence in cli.py.
kedro/framework/cli/cli.py
Outdated
@@ -98,12 +100,13 @@ def docs(): | |||
|
|||
def _init_plugins(): | |||
group = ENTRY_POINT_GROUPS["init"] | |||
for entry_point in pkg_resources.iter_entry_points(group=group): | |||
for entry_point in importlib_metadata.entry_points().select(group=group): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to refactor this function a bit so it uses load_entry_points
in utils.py? At a glance they look very similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original test for _init_plugins
is actually incorrectly testing the load
exception, it should be testing the error during hook initialisation instead.
init_hook = entry_point.load() # previously the the error was triggered here
init_hook() # this is what we want to test instead
tests/framework/cli/test_cli.py
Outdated
entry_point.module_name = "bob.fred" | ||
|
||
result = CliRunner().invoke(cli, ["info"]) | ||
print(result.output) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
print(result.output) |
pyproject.toml
Outdated
# PEP-518 https://peps.python.org/pep-0518/ | ||
[build-system] | ||
# Minimum requirements for the build system to execute. | ||
requires = ["setuptools>=38.0", "wheel"] # PEP 508 specifications. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've got "setuptools>=38.0"
and wheel
requirements in various other places in the repo (just do a Ctrl+F to see them). Does this mean we can remove them from there?
If yes then that would be nice but a bit of a bigger change, so probably worth doing in a separate PR.
Couples notes for the reviews:
|
Signed-off-by: noklam <nok.lam.chan@quantumblack.com>
Signed-off-by: noklam <nok.lam.chan@quantumblack.com>
Signed-off-by: noklam <nok.lam.chan@quantumblack.com>
Signed-off-by: noklam <nok.lam.chan@quantumblack.com>
Signed-off-by: noklam <nok.lam.chan@quantumblack.com>
…p-kedro-running-when-dependency-clashes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! ⭐ Thanks for the thorough description on the PR, that made it very easy to understand the changes made.
I'm happy to see the move to importlib_metadata
now pkg_resources
doesn't seem to be the proper standard anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really great work! Just one small comment. And also don't forget to add to RELEASE.md.
kedro/framework/cli/cli.py
Outdated
init_hook() | ||
except Exception as exc: | ||
raise KedroCliError(f"Initializing {entry_point}") from exc | ||
except Exception as exc: # pylint: disable=broad-except |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now you've made it clearer exactly what raises the exception here, I actually think we should remove the try
/except
altogether and just do a plain
for init_hook in init_hooks:
init_hooks()
Reasoning:
- exceptions raised by
entry_point.load()
are now handled byload_entry_points
- exceptions raised by
init_hook()
should still be raised, not hidden and logged, and there's not much point wrapping them inKedroCliError
like we used to. This is analogous to what happens e.g. if a plugin with a hook raises an exception - it doesn't get caught and logged or converted into a kedro error message
All this is kind of minor because realistically no one uses init_hooks
and it's probably redundant now we have before_command_run
anyway... But if it's not too annoying to chance I think this would be a small improvement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, I think it is actually wrong to catch the error, error should be raised!
Signed-off-by: noklam <nok.lam.chan@quantumblack.com>
…ct-error-stop-kedro-running-when-dependency-clashes' into fix/1487-contextualversionconflict-error-stop-kedro-running-when-dependency-clashes Signed-off-by: noklam <nok.lam.chan@quantumblack.com>
Signed-off-by: noklam nok.lam.chan@quantumblack.com
Description
Fix #1487
Currently, when using
kedro
with plugins, the conflict version between libraries will stop kedro completely to load. The more ideal way is to ignore the plugins and throw warnings.These errors can at least happen in 3 different ways.
kedro
itself. This could happen if you install kedro, then downgradepip
. In theory, if you use Python API it will run successfully, but anything involving the entry points will fail. The main reason is thatpkg_resource
always validates the dependencies, and there seems to be no easy way to fix it.kedro-xxxxx
plugins. This is the direct cause of the creation of this PR, the less intrusive way to deal with it is just to log the error instead of terminating the entire program. I choose to use a higher level of LOG level as for certain plugins, it could mean the run is actually no longer valid. (i.e.kedro-mlflow
or any plugin that actually has additional behavior for a kedro run.__main__
of a kedro package programmatically.Development notes
pkg_resource
withimportlib-xxx
for a few reasons.1.1 The
pkg_resource
is an old library and maybe deprecated soon, even the official website ofpkg_resource
suggest usingimportlib-xxx
alternatives whenever possible.pluggy
andpytest
replacedpkg_resource
withimportlib
too.1.2. Improve import speed - the
pkg_resource
import lots of thing at the top-level, which slow down the CLI. #14761.3. Reduce the chance that the
ContextualVersionConflict
types of error. This is not solved unfortunately, it seems that this validation still happens if one ofkedro
's dependencies usespkg_resource
.dynaconf
is one of them, and importingdynaconf
will trigger the same error.kedro
will log them instead of terminate the program.setuptools
topyproject.toml
(PEP-518, not really related to the PR but found out this when studying how the python build system works.)micropkg
is still usingpkg_resource
for requirement parsing, it is not handled now since it is out of scope.Compatibility issue:
https://bugs.python.org/issue44246
Unfortunately the entry_points() API is not consistent between Python version,
importlib.metadata
is only available with python>=3.8. I spend quite a while to figure this out and make all test passed. For now, the backward compatibleimportlib_metadata
seems to be a better option, but it is not with the standard libimportlib
.This is how it looks if the plugin's registration fails. For example I explicitly downgrade
![image](https://user-images.githubusercontent.com/18221871/169025429-292affb1-c06a-4963-bda0-bf3abf0bcc45.png)
plotly
sokedro-viz
is not registered successfully.Checklist
RELEASE.md
file